feat(server,cli): health, readiness, status, and preloop status - #172
feat(server,cli): health, readiness, status, and preloop status#172Bnjoroge1 wants to merge 7 commits into
Conversation
…cs, and preloop status
Entire-Checkpoint: 01M0GMAK0GBF4MWHPY34DVRZC3
Entire-Checkpoint: 01M0GMKC9BKEHVJBA4DXMRPFX6
Entire-Checkpoint: 01M0GZ9EAD3TT4DGQZDY03PBBX
preloop-vm and preloop-orchestrator share the fleet registry through preloop-observability instead of owning their own, avoiding a circular dependency. The host sampler itself is a stub until the cgroup/process parser lands. Entire-Checkpoint: 01M0GZN3CAJJM3NXHQ4Q9QHKC4
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| runs_req = runs_req.bearer_auth(token); | ||
| } | ||
| let runs: Vec<serde_json::Value> = match runs_req.send().await { | ||
| Ok(r) if r.status().is_success() => r.json().await.unwrap_or_default(), |
There was a problem hiding this comment.
🟡 Medium src/main.rs:2935
A malformed or truncated successful /api/v1/runs response is silently rendered as No runs found., falsely reporting an empty run history. unwrap_or_default() discards the body-read/JSON decoding error; handle that error explicitly, either by propagating it or emitting the same warning used by the other runs-fetch failure branches.
- Ok(r) if r.status().is_success() => r.json().await.unwrap_or_default(),
+ Ok(r) if r.status().is_success() => match r.json().await {
+ Ok(runs) => runs,
+ Err(e) => {
+ eprintln!("[warn] runs table unavailable: {e}");
+ Vec::new()
+ }
+ },🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-cli/src/main.rs around line 2935:
A malformed or truncated successful `/api/v1/runs` response is silently rendered as `No runs found.`, falsely reporting an empty run history. `unwrap_or_default()` discards the body-read/JSON decoding error; handle that error explicitly, either by propagating it or emitting the same warning used by the other runs-fetch failure branches.
| signal.store(true, std::sync::atomic::Ordering::Release); | ||
| } | ||
| if let Some(ps) = &self.config.pool_status { | ||
| ps.set_preparing(true); |
There was a problem hiding this comment.
🟡 Medium src/lib.rs:2263
When pool startup fails after set_preparing(true), PoolStatus.preparing remains true permanently, so the shared status reports that the pool is still warming even after the caller handles or retries the error. The ? returns from ensure_host_externals, artifact preparation, stale-machine cleanup, and golden setup bypass the only set_preparing(false) call; clear the flag on every exit, for example with a scope guard.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-orchestrator/src/lib.rs around line 2263:
When pool startup fails after `set_preparing(true)`, `PoolStatus.preparing` remains `true` permanently, so the shared status reports that the pool is still warming even after the caller handles or retries the error. The `?` returns from `ensure_host_externals`, artifact preparation, stale-machine cleanup, and golden setup bypass the only `set_preparing(false)` call; clear the flag on every exit, for example with a scope guard.
| return (StatusCode::SERVICE_UNAVAILABLE, Json(body)).into_response(); | ||
| } | ||
| // Check critical heartbeats freshness (>15s stale is 3 intervals of 5s sampler) | ||
| if let Some(stale) = shared |
There was a problem hiding this comment.
🟡 Medium src/runs.rs:24
readyz returns 200 even when required critical background tasks have never registered or have already exited. any_critical_stale returns None for an empty registry, and HeartbeatHandle deregistration removes exited tasks, so this check cannot distinguish healthy tasks from absent tasks. Require the expected critical tasks to be registered and running before reporting readiness.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-runner-server/src/runs.rs around line 24:
`readyz` returns 200 even when required critical background tasks have never registered or have already exited. `any_critical_stale` returns `None` for an empty registry, and `HeartbeatHandle` deregistration removes exited tasks, so this check cannot distinguish healthy tasks from absent tasks. Require the expected critical tasks to be registered and running before reporting readiness.
| heartbeat.beat("state_sampler"); | ||
| let mut interval = tokio::time::interval(Duration::from_secs(5)); | ||
| // Immediate sample then every 5s. | ||
| interval.tick().await; |
There was a problem hiding this comment.
🟡 Medium src/bootstrap.rs:565
The sampler skips its documented immediate sample, so /api/v1/status and /metrics serve the seeded all-zero snapshot for the first 5 seconds even when recovered state contains queued/running jobs or runners. interval.tick().await consumes Tokio's initial immediate tick before the loop; remove it so the select! handles that first tick and publishes the recovered state immediately.
- interval.tick().await;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-runner-server/src/bootstrap.rs around line 565:
The sampler skips its documented immediate sample, so `/api/v1/status` and `/metrics` serve the seeded all-zero snapshot for the first 5 seconds even when recovered state contains queued/running jobs or runners. `interval.tick().await` consumes Tokio's initial immediate tick before the loop; remove it so the `select!` handles that first tick and publishes the recovered state immediately.
| } else if pool_snapshot.preparing { | ||
| // Temporarily unclaimable while pool prepares. | ||
| (0, queue_jobs.len() as u32) | ||
| } else { |
There was a problem hiding this comment.
🟡 Medium src/bootstrap.rs:455
When pool_snapshot.preparing is true, the status snapshot reports every queued job as unclaimable, including jobs that an already registered matching runner can claim. The preparation branch bypasses the runner-label check; let claimability be determined by currently eligible runners instead.
- } else if pool_snapshot.preparing {
- // Temporarily unclaimable while pool prepares.
- (0, queue_jobs.len() as u32)
- } else {
+ } else {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-runner-server/src/bootstrap.rs around lines 455-458:
When `pool_snapshot.preparing` is true, the status snapshot reports every queued job as unclaimable, including jobs that an already registered matching runner can claim. The preparation branch bypasses the runner-label check; let claimability be determined by currently eligible runners instead.
| // must never run under the state lock. | ||
| let component = |name: &str, path: std::path::PathBuf| StorageComponent { | ||
| store: name.to_string(), | ||
| bytes: std::fs::metadata(&path).map(|m| m.len()).unwrap_or(0), |
There was a problem hiding this comment.
🟡 Medium src/bootstrap.rs:530
StorageSnapshot.components reports only the directory entry metadata size for cache and artifacts, not the bytes stored beneath them, so the CLI shows tiny and misleading storage totals even when those stores contain large data. Replace the direct metadata(...).len() calculation with a recursive directory-size calculation (while retaining the file metadata path for database).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-runner-server/src/bootstrap.rs around line 530:
`StorageSnapshot.components` reports only the directory entry metadata size for `cache` and `artifacts`, not the bytes stored beneath them, so the CLI shows tiny and misleading storage totals even when those stores contain large data. Replace the direct `metadata(...).len()` calculation with a recursive directory-size calculation (while retaining the file metadata path for `database`).
| fn default() -> Self { | ||
| Self { | ||
| mode: PoolMode::Warm, | ||
| desired: 0, |
There was a problem hiding this comment.
🟡 Medium src/status.rs:143
Active pools report desired, runner counts, provision failures, and last_transition_at as their default values (mostly zero or None) in /api/v1/status, even when runners are idle or busy. PoolStatus is only updated via set_preparing; the setters for these fields are never called, so passing pool_status here does not populate operational counters. Update it from the pool lifecycle transitions, including failure recording and clearing.
Also found in 1 other location(s)
crates/preloop-cli/src/main.rs:1746
Passing
pool_statushere does not actually wire the pool's operational counters into it. In the orchestrator, the onlyPoolStatusupdate isset_preparing; there are no calls toset_desired,set_counts,record_provision_failure, orclear_provision_failuresanywhere in the repository. Thus a running pool's status snapshot permanently reports the defaultdesired,building,provisioning,idle,busy,paused, and failure values (mostly zero), making the new pool status output materially incorrect.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-observability/src/status.rs around line 143:
Active pools report `desired`, runner counts, provision failures, and `last_transition_at` as their default values (mostly zero or `None`) in `/api/v1/status`, even when runners are idle or busy. `PoolStatus` is only updated via `set_preparing`; the setters for these fields are never called, so passing `pool_status` here does not populate operational counters. Update it from the pool lifecycle transitions, including failure recording and clearing.
Also found in 1 other location(s):
- crates/preloop-cli/src/main.rs:1746 -- Passing `pool_status` here does not actually wire the pool's operational counters into it. In the orchestrator, the only `PoolStatus` update is `set_preparing`; there are no calls to `set_desired`, `set_counts`, `record_provision_failure`, or `clear_provision_failures` anywhere in the repository. Thus a running pool's status snapshot permanently reports the default `desired`, `building`, `provisioning`, `idle`, `busy`, `paused`, and failure values (mostly zero), making the new pool status output materially incorrect.
| impl Default for PoolSnapshot { | ||
| fn default() -> Self { | ||
| Self { | ||
| mode: PoolMode::Warm, |
There was a problem hiding this comment.
🟡 Medium src/status.rs:142
/api/v1/status reports pool.mode as "warm" for disabled, on-demand, and external pools. PoolSnapshot::default() hard-codes mode to PoolMode::Warm, and the shared pool_status is never updated with the configured mode; initialize or update it from pool_enabled and the selected provisioning mode.
Also found in 1 other location(s)
crates/preloop-cli/src/main.rs:1746
pool_statusis passed through unchanged even though this function knowspool_enabledand whether provisioning is warm or on-demand. The shared handle is created fromPoolSnapshot::default()(mode: PoolMode::Warm), and no code anywhere updatesmode. Therefore disabling the warm pool still exposespool.mode = "warm"instead ofon_demand/disabled, misleading the new status endpoint about the pool's operating mode.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-observability/src/status.rs around line 142:
`/api/v1/status` reports `pool.mode` as `"warm"` for disabled, on-demand, and external pools. `PoolSnapshot::default()` hard-codes `mode` to `PoolMode::Warm`, and the shared `pool_status` is never updated with the configured mode; initialize or update it from `pool_enabled` and the selected provisioning mode.
Also found in 1 other location(s):
- crates/preloop-cli/src/main.rs:1746 -- `pool_status` is passed through unchanged even though this function knows `pool_enabled` and whether provisioning is warm or on-demand. The shared handle is created from `PoolSnapshot::default()` (`mode: PoolMode::Warm`), and no code anywhere updates `mode`. Therefore disabling the warm pool still exposes `pool.mode = "warm"` instead of `on_demand`/`disabled`, misleading the new status endpoint about the pool's operating mode.
| pool_preparing: None, | ||
| listen, | ||
| pool_status: None, | ||
| observability: None, |
There was a problem hiding this comment.
🟡 Medium src/main.rs:134
The standalone serve path passes None for observability, so AppState uses Observability::noop() and /api/v1/status, /metrics, heartbeat tracking, and limit telemetry are disconnected from the configured process handle. Pass the constructed handle into ServerConfig instead.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-runner-server/src/main.rs around line 134:
The standalone `serve` path passes `None` for `observability`, so `AppState` uses `Observability::noop()` and `/api/v1/status`, `/metrics`, heartbeat tracking, and limit telemetry are disconnected from the configured process handle. Pass the constructed handle into `ServerConfig` instead.
| pool_enabled, | ||
| pool_preparing.clone(), | ||
| pending_registrations.clone(), | ||
| pool_status.clone(), |
There was a problem hiding this comment.
🟡 Medium src/main.rs:1357
/api/v1/status reports pool.pending_registrations as zero while provision tokens are outstanding. pool_status and pending_registrations use independent maps; the server only copies the legacy map into PoolStatus once at startup, while provisioning later inserts and removes tokens only in RunnerPoolConfig::pending_registrations. Share the same token storage or synchronize every insertion and removal with PoolStatus.
Also found in 1 other location(s)
crates/preloop-observability/src/status.rs:174
PoolStatus::newcreates a second, independent pending-token map rather than sharing the authoritativepending_registrationsmap. The runner pool continues to insert tokens into the legacy map (there are no callers ofPoolStatus::insert_pendingoutside the one-time startup copy), so tokens created after startup never reach this map andsnapshot().pending_registrationsincorrectly remains zero while registrations are pending.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-cli/src/main.rs around line 1357:
`/api/v1/status` reports `pool.pending_registrations` as zero while provision tokens are outstanding. `pool_status` and `pending_registrations` use independent maps; the server only copies the legacy map into `PoolStatus` once at startup, while provisioning later inserts and removes tokens only in `RunnerPoolConfig::pending_registrations`. Share the same token storage or synchronize every insertion and removal with `PoolStatus`.
Also found in 1 other location(s):
- crates/preloop-observability/src/status.rs:174 -- `PoolStatus::new` creates a second, independent pending-token map rather than sharing the authoritative `pending_registrations` map. The runner pool continues to insert tokens into the legacy map (there are no callers of `PoolStatus::insert_pending` outside the one-time startup copy), so tokens created after startup never reach this map and `snapshot().pending_registrations` incorrectly remains zero while registrations are pending.
1 participant
Public
/healthz(503 during shutdown) and/readyz(503 on shutdown, stale critical heartbeats, or a stale sampler snapshot); bearer-auth/api/v1/statusand/metrics; a 5s off-lock state sampler producing theOperationalSnapshot; poolpreparing, storage, and GitHub state surfaced; OpenAPI docs. CLIpreloop statusbecomesstatus --json|--limitwith a 10-section human renderer and a/readyzengine probe. CLI fixes folded in: UTF-8-safetruncate_reason, the observability handle actually wired intoServerConfig/RunnerPoolConfig(wasNone), a typedOperationalSnapshotparse gate, and a real 2s shutdown flush.Part of a stacked series (merge bottom-up):
Summary by cubic
Expose health, readiness, metrics, and an operational status snapshot, and switch the CLI status to a readable report or raw JSON. Readiness now fails on shutdown, stale critical heartbeats, or a stale sampler; liveness returns 503 during shutdown.
Server
preloop-runner-server; OpenAPI updated.OperationalSnapshotevery 5s off-lock; caches it for /api/v1/status and Prometheus text at /metrics.preloop-observabilityinto AppState; registers heartbeats for the sampler, reaper, and scheduler; gates /readyz when critical tasks are >15s stale or the snapshot is stale.PoolStatus(preparing flag, desired, counts, queue depth, next_job_runs_on, pending tokens); legacy fields remain for compatibility.preloop-vmfor shared fleet accounting.CLI and migration
preloop-cli; prints a 10-section human report or raw snapshot JSON; probes /readyz with reason-aware timeouts.Written for commit 825727e. Summary will update on new commits.
Note
Add health, readiness, status, and /metrics endpoints to runner server
GET /readyz(public),GET /api/v1/status(auth), andGET /metrics(auth) to the runner server. A backgroundrun_state_samplerupdates a cachedOperationalSnapshotevery 5s in bootstrap.rs.GET /healthzto return 503 during shutdown.GET /readyzreturns 503 if critical heartbeats or the status snapshot are stale for >15s in runs.rs.statusto useStatusArgs(--json,--limit). It fetches/api/v1/statusand renders a multi-section report instead of single-run status in main.rs./readyzwith detailed timeout reasons.pool_statustoRunnerPoolConfigto sync thepreparingflag during pool warm-up in lib.rs.statusremoves positionalrun_idsupport.GET /healthzreturns 503 during shutdown. Engine bootstrap waits on/readyzinstead of/healthz.📊 Macroscope summarized 825727e. 12 files reviewed, 18 issues evaluated, 5 issues filtered, 12 comments posted
🗂️ Filtered Issues
crates/preloop-cli/src/main.rs — 2 comments posted, 5 evaluated, 2 filtered
pool_statusis passed through unchanged even though this function knowspool_enabledand whether provisioning is warm or on-demand. The shared handle is created fromPoolSnapshot::default()(mode: PoolMode::Warm), and no code anywhere updatesmode. Therefore disabling the warm pool still exposespool.mode = "warm"instead ofon_demand/disabled, misleading the new status endpoint about the pool's operating mode. [ Cross-file consolidated ]pool_statushere does not actually wire the pool's operational counters into it. In the orchestrator, the onlyPoolStatusupdate isset_preparing; there are no calls toset_desired,set_counts,record_provision_failure, orclear_provision_failuresanywhere in the repository. Thus a running pool's status snapshot permanently reports the defaultdesired,building,provisioning,idle,busy,paused, and failure values (mostly zero), making the new pool status output materially incorrect. [ Cross-file consolidated ]crates/preloop-observability/src/status.rs — 2 comments posted, 4 evaluated, 2 filtered
pending_registrationscannot reflect registrations created after startup. The legacy pending map is copied intoPoolStatusonly once during bootstrap, and repository-wide the only call toPoolStatus::insert_pendingis that initial copy whileremove_pendinghas no callers. Becausesnapshot()derives this field from the disconnectedpending_tokensmap,/api/v1/statusnormally continues reporting zero pending registrations while provisioning is underway. [ Out of scope ]PoolStatus::newcreates a second, independent pending-token map rather than sharing the authoritativepending_registrationsmap. The runner pool continues to insert tokens into the legacy map (there are no callers ofPoolStatus::insert_pendingoutside the one-time startup copy), so tokens created after startup never reach this map andsnapshot().pending_registrationsincorrectly remains zero while registrations are pending. [ Cross-file consolidated ]crates/preloop-runner-server/src/bootstrap.rs — 5 comments posted, 6 evaluated, 1 filtered
pool_preparingORs the legacy flag withpool_status.snapshot().preparing, but bootstrap copies atruelegacy value into the default privatePoolStatusonly once. For backward-compatible callers that providepool_preparingbut no sharedpool_status, clearing the atomic after warm-up leaves the copied snapshot permanentlytrue; every reaper pass then clearsqueued_at, so unmatched Linux jobs are never failed after the grace period and remain queued indefinitely. [ Out of scope ]